-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(imagebuilder-alpha): add support for Distribution Configuration Construct #36005
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
1de77fb to
cfde029
Compare
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
cfde029 to
9a9c664
Compare
Pull request has been modified.
1fba748 to
5d3ff75
Compare
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
Pull request has been modified.
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Show resolved
Hide resolved
| public addAmiDistributions(...amiDistributions: AmiDistribution[]): void { | ||
| amiDistributions.forEach((amiDistribution) => { | ||
| const region = amiDistribution.region ?? cdk.Stack.of(this).region; | ||
| if (!cdk.Token.isUnresolved(region) && this.amiDistributionsByRegion[region]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this restriction apply to a combination of AmiDistributionConfiguration and ContainerDistributionConfiguration being in same region well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct - each Distribution object must be unique to a region
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current validations only check for duplicate AMI distributions or duplicate Container distributions in the same region; there is no validation that prevents having both an AMI distribution and a Container distribution for the same region.
With the current implementation, if a user configures both an AMI and a Container distribution for the same region, we will have DistributionProperty for that region that has both amiDistributionConfiguration and containerDistributionConfiguration set. Is that intended?
Distributions: [
{
region: 'us-east-1',
amiDistributionConfiguration: { ... },
containerDistributionConfiguration: { ... },
...
}
....
]
packages/@aws-cdk/aws-imagebuilder-alpha/lib/distribution-configuration.ts
Outdated
Show resolved
Hide resolved
722fa15 to
bb3a318
Compare
Pull request has been modified.
… Distribution Configuration
bb3a318 to
5f7848e
Compare
| ), | ||
| ); | ||
| Object.values(this.containerDistributionsByRegion).forEach((containerDistribution) => { | ||
| const region = containerDistribution.region ?? cdk.Stack.of(this).region; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using the map key as region here as well similar to amiDistributionsByRegion, instead of recomputing region?
| throw new cdk.ValidationError('You must specify at least one AMI or container distribution', this); | ||
| } | ||
|
|
||
| const distributionByRegion: { [region: string]: CfnDistributionConfiguration.DistributionProperty } = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can simplify this and build the map with explicit loops, to make it more readable:
const distributionByRegion: { [region: string]: CfnDistributionConfiguration.DistributionProperty } = {};
for (const [region, distribution] of Object.entries(this.amiDistributionsByRegion)) {
distributionByRegion[region] = {
region,
amiDistributionConfiguration: this.buildAmiDistribution(distribution),
fastLaunchConfigurations: this.buildFastLaunchConfigurations(distribution),
launchTemplateConfigurations: this.buildLaunchTemplateConfigurations(distribution),
ssmParameterConfigurations: this.buildSsmParameterConfigurations(distribution),
licenseConfigurationArns: this.buildLicenseConfigurationArns(distribution),
};
}
for (const [region, containerDistribution] of Object.entries(this.containerDistributionsByRegion)) {
distributionByRegion[region] = {
...(distributionByRegion[region] ?? {}),
region,
containerDistributionConfiguration: this.buildContainerDistribution(containerDistribution),
};
}
| stack, | ||
| 'ContainerDistributionConfiguration', | ||
| { | ||
| description: 'This is an AMI distribution configuration.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| description: 'This is an AMI distribution configuration.', | |
| description: 'This is a Container distribution configuration.', |
Issue
aws/aws-cdk-rfcs#789
Reason for this change
This change adds a new alpha module for EC2 Image Builder L2 Constructs (
@aws-cdk/aws-imagebuilder-alpha), as outlined in aws/aws-cdk-rfcs#789. This PR specifically implements theDistributionConfigurationconstruct.Description of changes
This change implements the
DistributionConfigurationconstruct, which is a higher-level construct ofCfnDistributionConfiguration.Example
Describe any new or updated permissions being added
N/A - new L2 construct in alpha module
Description of how you validated changes
Validated with unit tests and integration tests. Manually verified generated CFN templates as well.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license